Skip to content
This repository was archived by the owner on Jun 30, 2025. It is now read-only.

Add http server. Closes #58#60

Closed
juliangruber wants to merge 2 commits intomainfrom
add/http-server
Closed

Add http server. Closes #58#60
juliangruber wants to merge 2 commits intomainfrom
add/http-server

Conversation

@juliangruber
Copy link
Copy Markdown
Member

Closes #58

@juliangruber juliangruber requested a review from bajtos March 20, 2023 13:46
Copy link
Copy Markdown
Member

@bajtos bajtos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good start! I quickly skimmed trough the changes, will take another look tomorrow.

Comment thread README.md

### `$ station --listen`

Open HTTP API.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this going to create another process providing only HTTP API for data created by another already running Station Core process? That seems like too many processes to me!

IMO, --listen should be an optional argument for the default command (station) that runs the main station process & modules.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe instead of --listen argument, we can implement the following flag:

  • If PORT is set to some value (including 0), we start the HTTP server
  • If PORT is not specified, we don't create any server.

It's less explicit than --listen, so maybe it's not a good idea. Posting it just for consideration.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, it's not a command it's just an option, meaning that it will run the basic $ station command, but also open an HTTP server.

IMO, --listen should be an optional argument for the default command (station) that runs the main station process & modules.

That's exactly how it is, I'm going to improve the README.

It's less explicit than --listen, so maybe it's not a good idea. Posting it just for consideration.

Those are my conerns as well. And we do read $PORT already, but only if --listen is set.

Comment thread README.md

Options:
-l, --listen Open HTTP API [boolean]
-p, --port HTTP API port [number] [default: 7834]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two options are relevant only to station; they don't apply to station metrics and friends, right?

I also don't see the --port argument defined anywhere in bin/station.js.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, that's outdated. Will fix!

@juliangruber juliangruber mentioned this pull request Mar 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

--listen, --port

2 participants